Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normalize opaque types with bound vars...... again #107620

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Feb 3, 2023

Fixes #107557, which demonstrates a real-life linker error due to not normalizing opaques with escaping bound vars.
Also fixes #109464.

I'm gonna run a crater run to see if we have any crates go from success -> timeout. Hopefully that'll help us better recognize if there are any major regressions? I may then be able to integrate some of my normalization work from #104133 to bring the compile times down for these crates...

I don't expect to be able to land this even if the crater run is clean. I'll still need to check that one crate that had regressed back when I tried to land this previously.

r? @ghost

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 3, 2023
@compiler-errors
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Feb 3, 2023

⌛ Trying commit a96733ce878652e60c549bf14ce38610bd49cf9d with merge 0179c3cb935eb588d407cb784f436ae84d99db5c...

@bors
Copy link
Contributor

bors commented Feb 3, 2023

☀️ Try build successful - checks-actions
Build commit: 0179c3cb935eb588d407cb784f436ae84d99db5c (0179c3cb935eb588d407cb784f436ae84d99db5c)

@compiler-errors
Copy link
Member Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-107620 created and queued.
🤖 Automatically detected try build 0179c3cb935eb588d407cb784f436ae84d99db5c
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 3, 2023
@compiler-errors
Copy link
Member Author

@craterbot cancel

@craterbot
Copy link
Collaborator

🚨 Error: failed to parse the command

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@compiler-errors
Copy link
Member Author

@craterbot run mode=build-only

@craterbot
Copy link
Collaborator

👌 Experiment pr-107620-1 created and queued.
🤖 Automatically detected try build 0179c3cb935eb588d407cb784f436ae84d99db5c
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@compiler-errors
Copy link
Member Author

@craterbot abort name=pr-107620

@craterbot
Copy link
Collaborator

🗑️ Experiment pr-107620 deleted!

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Feb 3, 2023
@craterbot
Copy link
Collaborator

🚧 Experiment pr-107620-1 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@jackh726
Copy link
Member

jackh726 commented Feb 3, 2023

What a cursed change. Good luck.

Copy link
Member Author

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, should remove comments while I'm at it.

compiler/rustc_trait_selection/src/traits/project.rs Outdated Show resolved Hide resolved
@craterbot
Copy link
Collaborator

🎉 Experiment pr-107620-1 is completed!
📊 37 regressed and 13 fixed (254324 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@Dylan-DPC Dylan-DPC added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 19, 2023
@rust-cloud-vms rust-cloud-vms bot force-pushed the normalize-opaques-with-late-bounds-again branch from fffca29 to 3a79842 Compare June 21, 2023 02:23
@rustbot rustbot added the WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) label Jun 21, 2023
@rust-cloud-vms rust-cloud-vms bot force-pushed the normalize-opaques-with-late-bounds-again branch from 3a79842 to 720477f Compare June 21, 2023 02:24
@compiler-errors
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. labels Jun 21, 2023
@craterbot
Copy link
Collaborator

🚧 Experiment pr-107620 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@rust-cloud-vms rust-cloud-vms bot force-pushed the normalize-opaques-with-late-bounds-again branch from 720477f to 89b56d9 Compare June 23, 2023 21:55
@craterbot
Copy link
Collaborator

🎉 Experiment pr-107620 is completed!
📊 92 regressed and 6 fixed (304944 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jun 24, 2023
@compiler-errors
Copy link
Member Author

@craterbot run mode=check-only p=1 crates=https://crater-reports.s3.amazonaws.com/pr-107620/retry-regressed-list.txt

(p=1 because it's a really small list, shouldn't noticeably delay the other experiments queued)

@craterbot
Copy link
Collaborator

👌 Experiment pr-107620-2 created and queued.
🤖 Automatically detected try build 9e1116444e23dde580a35d0bf5bbb46c1ce9ce25
⚠️ Try build based on commit 720477ffbdf1d3a2c28ba76a3095e55ceb198482, but latest commit is 89b56d9. Did you forget to make a new try build?
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 24, 2023
@craterbot
Copy link
Collaborator

🚧 Experiment pr-107620-2 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-107620-2 is completed!
📊 2 regressed and 0 fixed (92 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jun 25, 2023
@compiler-errors
Copy link
Member Author

Closing this to clean up and put in a new PR

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 29, 2023
…th-late-bound-vars-again, r=jackh726

Normalize opaques with late-bound vars again

We have a hack in the compiler where if an opaque has escaping late-bound vars, we skip revealing it even though we *could* reveal it from a technical perspective. First of all, this is weird, since we really should be revealing all opaques in `Reveal::All` mode. Second of all, it causes subtle bugs (linked below).

I attempted to fix this in rust-lang#100980, which was unfortunately reverted due to perf regressions on codebases that used really deeply nested futures in some interesting ways. The worst of which was rust-lang#103423, which caused the project to hang on build. Another one was rust-lang#104842, which was just a slow-down, but not a hang. I took some time afterwards to investigate how to rework `normalize_erasing_regions` to take advantage of better caching, but that effort kinda fizzled out (rust-lang#104133).

However, recently, I was made aware of more bugs whose root cause is not revealing opaques during codegen. That made me want to fix this again -- in the process, interestingly, I took the the minimized example from rust-lang#103423 (comment), and it doesn't seem to hang any more...

Thinking about this harder, there have been some changes to the way we lower and typecheck async futures that may have reduced the pathologically large number of outlives obligations (see description of rust-lang#103423) that we were encountering when normalizing opaques with bound vars the last time around:
* rust-lang#104321 (lower `async { .. }` directly as a generator that implements `Future`, removing the `from_generator` shim)
* rust-lang#104833 (removing an `identity_future` fn that was wrapping desugared future generators)

... so given that I can see:
* No significant regression on rust perf bot (rust-lang#107620 (comment))
* No timeouts in crater run I did (rust-lang#107620 (comment), rechecked failing crates in rust-lang#107620 (comment))

... and given that this PR:
* Fixes rust-lang#104601
* Fixes rust-lang#107557
* Fixes rust-lang#109464
* Allows us to remove a `DefiningAnchor::Bubble` from codegen (75a8f68)

I'm inclined to give this another shot at landing this. Best case, it just works -- worst case, we get more examples to study how we need to improve the compiler to make this work.

r? types
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Jul 2, 2023
…ound-vars-again, r=jackh726

Normalize opaques with late-bound vars again

We have a hack in the compiler where if an opaque has escaping late-bound vars, we skip revealing it even though we *could* reveal it from a technical perspective. First of all, this is weird, since we really should be revealing all opaques in `Reveal::All` mode. Second of all, it causes subtle bugs (linked below).

I attempted to fix this in #100980, which was unfortunately reverted due to perf regressions on codebases that used really deeply nested futures in some interesting ways. The worst of which was #103423, which caused the project to hang on build. Another one was #104842, which was just a slow-down, but not a hang. I took some time afterwards to investigate how to rework `normalize_erasing_regions` to take advantage of better caching, but that effort kinda fizzled out (#104133).

However, recently, I was made aware of more bugs whose root cause is not revealing opaques during codegen. That made me want to fix this again -- in the process, interestingly, I took the the minimized example from rust-lang/rust#103423 (comment), and it doesn't seem to hang any more...

Thinking about this harder, there have been some changes to the way we lower and typecheck async futures that may have reduced the pathologically large number of outlives obligations (see description of #103423) that we were encountering when normalizing opaques with bound vars the last time around:
* #104321 (lower `async { .. }` directly as a generator that implements `Future`, removing the `from_generator` shim)
* #104833 (removing an `identity_future` fn that was wrapping desugared future generators)

... so given that I can see:
* No significant regression on rust perf bot (rust-lang/rust#107620 (comment))
* No timeouts in crater run I did (rust-lang/rust#107620 (comment), rechecked failing crates in rust-lang/rust#107620 (comment))

... and given that this PR:
* Fixes #104601
* Fixes #107557
* Fixes #109464
* Allows us to remove a `DefiningAnchor::Bubble` from codegen (75a8f681837c70051e0200a14f58ae07dbe58e66)

I'm inclined to give this another shot at landing this. Best case, it just works -- worst case, we get more examples to study how we need to improve the compiler to make this work.

r? types
@compiler-errors compiler-errors deleted the normalize-opaques-with-late-bounds-again branch August 11, 2023 20:11
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Apr 7, 2024
…ound-vars-again, r=jackh726

Normalize opaques with late-bound vars again

We have a hack in the compiler where if an opaque has escaping late-bound vars, we skip revealing it even though we *could* reveal it from a technical perspective. First of all, this is weird, since we really should be revealing all opaques in `Reveal::All` mode. Second of all, it causes subtle bugs (linked below).

I attempted to fix this in #100980, which was unfortunately reverted due to perf regressions on codebases that used really deeply nested futures in some interesting ways. The worst of which was #103423, which caused the project to hang on build. Another one was #104842, which was just a slow-down, but not a hang. I took some time afterwards to investigate how to rework `normalize_erasing_regions` to take advantage of better caching, but that effort kinda fizzled out (#104133).

However, recently, I was made aware of more bugs whose root cause is not revealing opaques during codegen. That made me want to fix this again -- in the process, interestingly, I took the the minimized example from rust-lang/rust#103423 (comment), and it doesn't seem to hang any more...

Thinking about this harder, there have been some changes to the way we lower and typecheck async futures that may have reduced the pathologically large number of outlives obligations (see description of #103423) that we were encountering when normalizing opaques with bound vars the last time around:
* #104321 (lower `async { .. }` directly as a generator that implements `Future`, removing the `from_generator` shim)
* #104833 (removing an `identity_future` fn that was wrapping desugared future generators)

... so given that I can see:
* No significant regression on rust perf bot (rust-lang/rust#107620 (comment))
* No timeouts in crater run I did (rust-lang/rust#107620 (comment), rechecked failing crates in rust-lang/rust#107620 (comment))

... and given that this PR:
* Fixes #104601
* Fixes #107557
* Fixes #109464
* Allows us to remove a `DefiningAnchor::Bubble` from codegen (75a8f681837c70051e0200a14f58ae07dbe58e66)

I'm inclined to give this another shot at landing this. Best case, it just works -- worst case, we get more examples to study how we need to improve the compiler to make this work.

r? types
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
…ound-vars-again, r=jackh726

Normalize opaques with late-bound vars again

We have a hack in the compiler where if an opaque has escaping late-bound vars, we skip revealing it even though we *could* reveal it from a technical perspective. First of all, this is weird, since we really should be revealing all opaques in `Reveal::All` mode. Second of all, it causes subtle bugs (linked below).

I attempted to fix this in #100980, which was unfortunately reverted due to perf regressions on codebases that used really deeply nested futures in some interesting ways. The worst of which was #103423, which caused the project to hang on build. Another one was #104842, which was just a slow-down, but not a hang. I took some time afterwards to investigate how to rework `normalize_erasing_regions` to take advantage of better caching, but that effort kinda fizzled out (#104133).

However, recently, I was made aware of more bugs whose root cause is not revealing opaques during codegen. That made me want to fix this again -- in the process, interestingly, I took the the minimized example from rust-lang/rust#103423 (comment), and it doesn't seem to hang any more...

Thinking about this harder, there have been some changes to the way we lower and typecheck async futures that may have reduced the pathologically large number of outlives obligations (see description of #103423) that we were encountering when normalizing opaques with bound vars the last time around:
* #104321 (lower `async { .. }` directly as a generator that implements `Future`, removing the `from_generator` shim)
* #104833 (removing an `identity_future` fn that was wrapping desugared future generators)

... so given that I can see:
* No significant regression on rust perf bot (rust-lang/rust#107620 (comment))
* No timeouts in crater run I did (rust-lang/rust#107620 (comment), rechecked failing crates in rust-lang/rust#107620 (comment))

... and given that this PR:
* Fixes #104601
* Fixes #107557
* Fixes #109464
* Allows us to remove a `DefiningAnchor::Bubble` from codegen (75a8f681837c70051e0200a14f58ae07dbe58e66)

I'm inclined to give this another shot at landing this. Best case, it just works -- worst case, we get more examples to study how we need to improve the compiler to make this work.

r? types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
7 participants